Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add checker merge feature #2120

Closed
wants to merge 1 commit into from
Closed

Conversation

wangxp006
Copy link
Contributor

No description provided.

@wangxp006 wangxp006 closed this by deleting the head repository Jan 2, 2025
Copy link
Collaborator

@pqarmitage pqarmitage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a number of issues with this patch:

  1. It doesn't allow for different types of checker - checkers will be merged even if they are of different types.
  2. So far as I can see the patch assumes that there will be only 1 non-merged checker per RS (rs->checker implies this).
  3. The hash is generated only from the IP address of the real server. This therefore means that all checkers for any particular real server will be merged (this perhaps explains why point 2 above has been implemented). The hash needs to be generated from parameters of the checker itself, and the real server address is irrelevant.
  4. The checker u_list is unused.
  5. If RS_CHECK_MERGE_BITS is less than 16 some of the bits in addr_fold will be unused. The code should be:
hash = 0;
while (addr_fold) {
    hash ^= addr_fold & RS_CHECK_MERGE_MASK;
    addr_fold >>= RS_CHECK_MERGE_BITS;
}
  1. rs_check_merge[RS_CHECK_MERGE_ENTRY_NUM] is huge (it is 8Mb on a 64-bit machine and half that on a 32-bit system) and it would need a very large number of checkers to be efficient.
  2. The checker h_list and i_list only need a single list_head entry in the checker structure since a checker is moved from the h_list to the i_list when it is found to be a duplicate, and the checker can never be on both lists.

Having said the above, I like the approach. I will modify the patch in line with the following:

  1. The hash will be generated from the parameters of the checker, including the type. We already have a compare function for each checker type so that can be used to compare the checkers if the hash matches.
  2. Rather than use the rs_check_merge array of list_heads we will use a red-black tree.
  3. Modify the checker i_list so that it is a list of all the RSs that use the checker, rather than having the first one accessed via checker->rs and the subsequent RSs via the i_list.
  4. Release the duplicate checkers (i.e. those that were added to the i_list), since they contain identical information to the first one found, and simply maintain a list of RSs that the checker updates. This is similar to the way BFD_CHECK and FILE_CHECK work, where the checkers are defined outside the scope of the virtual/real servers, and the checker configuration references the separate track_file/bfd_instance.
  5. We need to take account of ha_suspend.
  6. Allow multiple checkers per RS.
  7. Update migrate_checkers(), used when reloading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants